12073: ProductRepository fails to load product with camelcase SKU #12169

RomaKis · 2017-11-10T09:49:42Z

ProductRepository fails to load product with camelcase SKU

Fixed Issues (if relevant)

ProductRepository fails to load product with camelcase SKU #12073: ProductRepository fails to load product with camelcase SKU

Manual testing scenarios

Create a simple product with the camel case sku (for example "Test")
Use get() method of \Magento\Catalog\Model\ProductRepository to get this product by sku, and specify the sku in lower case.

Expected result

NoSuchEntityException is returned

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

orlangur · 2017-11-10T17:11:25Z

app/code/Magento/Catalog/Model/ProductRepository.php

        }
+
+        if (!isset($this->instances[$sku])) {
+            throw new NoSuchEntityException(__('Requested product doesn\'t exist'));


This code completely duplicates previous condition.

What is this issue all about? I see a trim but don't see any strtolower in logic like $this->instances[$product->getSku()][$cacheKey] = $product;.

@orlangur, ok, let me explain. We have product with sku "Test". We try to use "get" method of repository to get product with sku "test". Method resourceModel->getIdBySku($sku) will return to us id of "Test" and in $this->instances it put the sku "Test". So after trim we are checking if we have $this->instances["test"] and we don't have it, because we have $this->instances["Test"].

How? https://3v4l.org/smbkn

@orlangur, what how? Trimming isn't my logic). It was before my changes. I only check if product sku("Test") is the same as requested sku("test"). And if it isn't the same exception will be thrown.

Ok, let me reword. Why product is stored with lower-cased SKU in the first place?

We must find a root cause so that exception will be thrown from line 240 instead of adding more exceptions throwing.

RomaKis · 2017-12-14T08:38:48Z

@orlangur, any updates on it?

RomaKis · 2018-01-03T14:44:08Z

@orlangur, ping.

orlangur · 2018-01-10T22:44:35Z

app/code/Magento/Catalog/Model/ProductRepository.php

        }
+
+        if (!isset($this->instances[$sku])) {
+            throw new NoSuchEntityException(__('Requested product doesn\'t exist'));


Ok, let me reword. Why product is stored with lower-cased SKU in the first place?

We must find a root cause so that exception will be thrown from line 240 instead of adding more exceptions throwing.

erikhansen · 2018-02-20T06:46:18Z

For the record, this PR will not fix the issue that it purports to fix (#12073). It will simply result in an exception rather than an error if the condition described in #12073 is met. However it will not allow a SKU with a mixed-case to get loaded via the product repository.

See this comment for more context.

orlangur · 2018-02-21T11:25:12Z

@erikhansen,

this PR will not fix the issue that it purports to fix (#12073)

Expected result
NoSuchEntityException is returned

Actually, not, but I do agree with comment you referring to that it is better to fix regression introduced.

orlangur

@RomaKis,

It's a regression in 2.2.0, still present as of 2.2.2. The problem was introduced right here: cb3b5d1#diff-ff4893a5628d34910295bae32fbcddfdL241

Just change implementation of cacheProduct in such a way that we pass both productId and sku instead of obtaining them from product instance.

The test should enforce that we can actually load product by such SKU.

RomaKis · 2018-02-26T08:32:02Z

@orlangur

Just change implementation of cacheProduct in such a way that we pass both productId and sku instead of obtaining them from product instance.

This sounds obscure, because we have two different functions get() and getById() which have only sku or only id as argument. Or do you mean to create two separate methods cacheBySku and cacheById ? In that way product will be cashed after first load only by by id or only by sku, but not by both.
Also I think this is not good idea, because we can ask product by sku test and Test. First it will cache it with the key test and the second time it will cache it again with the key Test, so we will have two same products in cache with different keys.

orlangur · 2018-03-09T12:34:36Z

@RomaKis,

so we will have two same products in cache with different keys.

That's the point. It should work this way.

Just rework implementation of cacheProduct in cb3b5d1#diff-ff4893a5628d34910295bae32fbcddfdL241 so that code behavior remain unchanged.

RomaKis · 2018-03-09T13:57:33Z

@orlangur, Why do we need to cache "test" and "Test" if we have only only one product? It is obscure. This is misleading and I don't think this is right.
I think we have two ways to resolve this issue:

make all skus strtolower();
throw exception if request sku is wrong.

orlangur · 2018-03-09T21:17:23Z

It is obscure. This is misleading and I don't think this is right.

Please reword a bit. strtolower implementation is a bit fragile and counter-intuitive. Proposed approach does not lead to any negative side effect as there is already mechanism to prevent cache overflow.

RomaKis · 2018-03-12T10:54:51Z

You are right that strtolower isn't a good solution.

So, I think it should throw exception. If I have product with sku "Test", so why when I ask product with sku "teSt" it should return the same product? We don't have product with sku "teSt", we have only with sku "Test". This is misleading for me.

orlangur · 2018-03-13T06:00:58Z

@RomaKis because that's how it worked before we introduced caching in product repository.

Another case I didn't mention is that MySQL does not distinguish not just lower/upper case but also е and ё, o and ö, a and ã and so on.

There is no need to introduce any additional restrictions if we can keep the things consistent without them.

RomaKis · 2018-03-14T09:27:05Z

If it worked in such way it didn't mean that it worked right. I remain at my mind - Magento shouldn't cache products with non-existent sku. As I mentioned it is misleading.

You can SELECT from SQL case sensitive with BINARY.

If you don't accept this implementation - so close this PR )

orlangur · 2018-03-14T09:49:06Z

Such case is not nonexistent SKU in fact and is a behavior explicitly requested in #12169 (comment).

mcspronko · 2018-07-23T07:34:48Z

@orlangur What is the reason for rejecting the pull request? The issue is from Nov 2017 and still not fixed. Is there a plan in place to address this issue or it is up to the community and their availability to fix it?

orlangur · 2018-08-02T13:41:02Z

@mcspronko because a proper implementation approach is performed in #14019. It needs some assistance with test but no time to look into it yet.

Cc: @maximgubar

12073: ProductRepository fails to load product with camelcase SKU

a2564bc

magento-engcom-team added Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Nov 10, 2017

ishakhsuvarov self-assigned this Nov 10, 2017

ishakhsuvarov added this to the November 2017 milestone Nov 10, 2017

ishakhsuvarov added the Release Line: 2.2 label Nov 10, 2017

orlangur requested changes Nov 10, 2017

View reviewed changes

ishakhsuvarov assigned orlangur and unassigned ishakhsuvarov Nov 14, 2017

okorshenko modified the milestones: November 2017, December 2017 Dec 1, 2017

okorshenko modified the milestones: December 2017, January 2018 Jan 8, 2018

orlangur requested changes Jan 10, 2018

View reviewed changes

orlangur added the Progress: needs update label Jan 10, 2018

okorshenko modified the milestones: January 2018, February 2018 Feb 7, 2018

orlangur requested changes Feb 21, 2018

View reviewed changes

okorshenko modified the milestones: February 2018, March 2018 Mar 1, 2018

orlangur mentioned this pull request Mar 9, 2018

12073 product repo get different sku case #14019

Closed

4 tasks

orlangur closed this Mar 14, 2018

orlangur added the Progress: reject label Mar 14, 2018

ghost mentioned this pull request Feb 4, 2019

ProductRepository fails to load product with camelcase SKU #12073

Closed

12073: ProductRepository fails to load product with camelcase SKU #12169

12073: ProductRepository fails to load product with camelcase SKU #12169

Uh oh!

Conversation

RomaKis commented Nov 10, 2017

Fixed Issues (if relevant)

Manual testing scenarios

Expected result

Contribution checklist

Uh oh!

orlangur Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

RomaKis Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

orlangur Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

RomaKis Nov 11, 2017

Choose a reason for hiding this comment

Uh oh!

orlangur Jan 10, 2018

Choose a reason for hiding this comment

Uh oh!

RomaKis commented Dec 14, 2017

Uh oh!

RomaKis commented Jan 3, 2018

Uh oh!

orlangur Jan 10, 2018

Choose a reason for hiding this comment

Uh oh!

erikhansen commented Feb 20, 2018

Uh oh!

orlangur commented Feb 21, 2018

Uh oh!

orlangur left a comment

Choose a reason for hiding this comment

Uh oh!

RomaKis commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orlangur commented Mar 9, 2018

Uh oh!

RomaKis commented Mar 9, 2018

Uh oh!

orlangur commented Mar 9, 2018

Uh oh!

RomaKis commented Mar 12, 2018

Uh oh!

orlangur commented Mar 13, 2018

Uh oh!

RomaKis commented Mar 14, 2018

Uh oh!

orlangur commented Mar 14, 2018

Uh oh!

mcspronko commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orlangur commented Aug 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

RomaKis commented Feb 26, 2018 •

edited

Loading

mcspronko commented Jul 23, 2018 •

edited

Loading